Skip to content

Update keepalive field for http upstreams #311

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Jul 11, 2024
Merged

Update keepalive field for http upstreams #311

merged 4 commits into from
Jul 11, 2024

Conversation

dhurley
Copy link
Contributor

@dhurley dhurley commented Jul 5, 2024

Proposed changes

According to the docs here https://nginx.org/en/docs/http/ngx_http_api_module.html#def_nginx_http_upstream, the keepalive field is called keepalive not keepalives.
This PR fixes the field name in the Upstream struct.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

@dhurley dhurley requested a review from a team as a code owner July 5, 2024 15:23
@dhurley dhurley changed the title fix: update keepalive field for http upstreams Update keepalive field for http upstreams Jul 8, 2024
@lucacome
Copy link
Contributor

lucacome commented Jul 8, 2024

@dhurley are you not able to see the number of keepalive without this fix? I don't see how it was able to unmarshal it with the wrong field name 🤔

@dhurley
Copy link
Contributor Author

dhurley commented Jul 9, 2024

@dhurley are you not able to see the number of keepalive without this fix? I don't see how it was able to unmarshal it with the wrong field name 🤔

From my testing, if a field defined in your struct that isn't in the json payload then it just defaults to zero. It doesn't error out.

@lucacome
Copy link
Contributor

lucacome commented Jul 9, 2024

From my testing, if a field defined in your struct that isn't in the json payload then it just defaults to zero. It doesn't error out.

Sorry I didn't phrase it the best way, I was just wondering if you confirmed that this never worked before (since it always had the extra s) 😅

@dhurley
Copy link
Contributor Author

dhurley commented Jul 10, 2024

From my testing, if a field defined in your struct that isn't in the json payload then it just defaults to zero. It doesn't error out.

Sorry I didn't phrase it the best way, I was just wondering if you confirmed that this never worked before (since it always had the extra s) 😅

Yes I can confirm it never worked. Here is an example curl request I made against the Plus API confirming that it is keepalive and not keepalives.

curl localhost:80/api/9/http/upstreams/ | jq
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  1390  100  1390    0     0  1357k      0 --:--:-- --:--:-- --:--:-- 1357k
{
  "test_backend": {
    "peers": [
      {
        "id": 0,
        "server": "[2a00:1450:4001:827::200e]:80",
        "name": "google.com:80",
        "backup": false,
        "weight": 1,
        "state": "up",
        "active": 0,
        "requests": 0,
        "responses": {
          "1xx": 0,
          "2xx": 0,
          "3xx": 0,
          "4xx": 0,
          "5xx": 0,
          "codes": {},
          "total": 0
        },
        "sent": 0,
        "received": 0,
        "fails": 0,
        "unavail": 0,
        "health_checks": {
          "checks": 0,
          "fails": 0,
          "unhealthy": 0
        },
        "downtime": 0
      },
      {
        "id": 1,
        "server": "172.217.16.206:80",
        "name": "google.com:80",
        "backup": false,
        "weight": 1,
        "state": "up",
        "active": 0,
        "requests": 0,
        "responses": {
          "1xx": 0,
          "2xx": 0,
          "3xx": 0,
          "4xx": 0,
          "5xx": 0,
          "codes": {},
          "total": 0
        },
        "sent": 0,
        "received": 0,
        "fails": 0,
        "unavail": 0,
        "health_checks": {
          "checks": 0,
          "fails": 0,
          "unhealthy": 0
        },
        "downtime": 0
      },
      {
        "id": 2,
        "server": "[2a00:1450:4001:81c::2003]:80",
        "name": "google.ie:80",
        "backup": false,
        "weight": 1,
        "state": "up",
        "active": 0,
        "requests": 0,
        "responses": {
          "1xx": 0,
          "2xx": 0,
          "3xx": 0,
          "4xx": 0,
          "5xx": 0,
          "codes": {},
          "total": 0
        },
        "sent": 0,
        "received": 0,
        "fails": 0,
        "unavail": 0,
        "health_checks": {
          "checks": 0,
          "fails": 0,
          "unhealthy": 0
        },
        "downtime": 0
      },
      {
        "id": 3,
        "server": "142.250.185.67:80",
        "name": "google.ie:80",
        "backup": false,
        "weight": 1,
        "state": "up",
        "active": 0,
        "requests": 0,
        "responses": {
          "1xx": 0,
          "2xx": 0,
          "3xx": 0,
          "4xx": 0,
          "5xx": 0,
          "codes": {},
          "total": 0
        },
        "sent": 0,
        "received": 0,
        "fails": 0,
        "unavail": 0,
        "health_checks": {
          "checks": 0,
          "fails": 0,
          "unhealthy": 0
        },
        "downtime": 0
      }
    ],
    "keepalive": 0,
    "queue": {
      "size": 0,
      "max_size": 10,
      "overflows": 0
    },
    "zombies": 0,
    "zone": "my_stream_backend3"
  }
}

@lucacome lucacome added the bug An issue reporting a potential bug label Jul 10, 2024
@lucacome lucacome added this to the v1.2.2 milestone Jul 10, 2024
@lucacome lucacome mentioned this pull request Jul 10, 2024
Closed
@github-actions github-actions bot removed the bug An issue reporting a potential bug label Jul 11, 2024
@oliveromahony
Copy link
Contributor

@lucacome is this ok to be merged?

@lucacome lucacome added the bug An issue reporting a potential bug label Jul 11, 2024
@lucacome
Copy link
Contributor

@oliveromahony it's ok for me

@oliveromahony oliveromahony merged commit 56570a6 into nginx:main Jul 11, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliveromahony oliveromahony oliveromahony approved these changes

@lucacome lucacome lucacome approved these changes

@haywoodsh haywoodsh haywoodsh approved these changes

@vepatel vepatel vepatel approved these changes

@aphralG aphralG aphralG approved these changes

Assignees
No one assigned
Labels
bug An issue reporting a potential bug
Projects
None yet
Milestone
v1.2.2
Development

Successfully merging this pull request may close these issues.
6 participants
@dhurley @lucacome @oliveromahony @haywoodsh @vepatel @aphralG